Skip to content

Convert remaining components to CSS logical properties (#1084) - #1174

Open
stephaniehobson wants to merge 2 commits into
v23/logical-templatesfrom
v23/logical-components
Open

stephaniehobson wants to merge 2 commits into
v23/logical-templatesfrom
v23/logical-components

Conversation

@stephaniehobson

@stephaniehobson stephaniehobson commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Description

Convert remaining components to CSS logical properties (#1084)

  • I have documented this change in the design system
  • I have recorded this change in CHANGELOG.md.

Issue

#1084

Testing

Poke around 🤷‍♀️

Visual regression tests found no problems.


Stack created with GitHub Stacks CLI • Give Feedback 💬

First pass of the logical-properties migration -- establishes the
conversion pattern the rest of the workstream follows, and removes
23 of the 127 remaining @include bidi() calls.

  includes/mixins/_utils.scss     (0 bidi, 2 physical: text-align kept
                                    physical -- see note below; inset:0
                                    shorthand for a symmetric all-sides
                                    absolute-position reset)
  includes/mixins/_details.scss   (2 bidi -> padding-inline-end,
                                    inset-inline-end)
  includes/forms/index.scss       (1 bidi -> inset-inline-start;
                                    this was the form-msg-pointer
                                    mixed-decls warning noted as
                                    deferred back in the mixed-decls
                                    PR -- confirmed fixed)
  base/elements/_lists.scss       (8 bidi -> margin-inline-start,
                                    all identical margin-left/right
                                    swap pattern; also collapsed two
                                    margin-left+margin-right !important
                                    pairs to margin-inline !important)
  base/elements/_forms.scss       (2 bidi -> padding-inline-end; the
                                    select rule's background-position
                                    kept physical with an explicit
                                    [dir='rtl'] override -- background-
                                    position has no standard logical
                                    keyword syntax safe for this
                                    matrix -- but its accompanying
                                    padding tuple did convert cleanly
                                    to padding-block + padding-inline)
  base/elements/_links.scss       (2 bidi -> margin-inline shorthand)
  base/elements/_tables.scss      (2 bidi -> text-align: start; also
                                    removed a redundant plain
                                    text-align: left that duplicated
                                    the LTR half of the old bidi call)
  base/elements/_quotes.scss      (1 bidi -> border-block-width +
                                    border-inline-*-width, since there's
                                    no single logical shorthand for all
                                    four border-width sides at once)
  base/elements/_details.scss     (1 bidi -> padding-inline, symmetric
                                    zero on both sides)
  base/utilities/_rich-text.scss  (4 bidi -> margin-inline-start, same
                                    swap pattern as _lists.scss)

Left _utils.scss's image-replaced mixin's text-align: left alone --
it also hardcodes direction: ltr, so it's intentionally fixed
regardless of page direction (an old image-replacement technique for
hiding text completely), not a case of missing RTL support. Using
text-align: start there would be misleading, implying adaptiveness
that was deliberately designed out.

Caught a real bug before it shipped: moving @include forms.form-input
to the end of the select rule would have fully silenced its last 2
mixed-decls warnings, but form-input() also sets a plain
padding: $field-padding shorthand -- moving it after my new
padding-inline/padding-block would let that shorthand win the cascade
and silently remove the space reserved for the dropdown caret icon.
Reverted to form-input's original early position; the 2 warnings stay
deferred (same conclusion the mixed-decls PR reached), but the actual
rendered padding is unaffected.

Found a second, related bug via a real visual regression report
(label.mzp-u-inline in _forms.scss): the original bidi() call used a
full 4-value padding shorthand (0 $spacing-sm 0 0), which explicitly
zeroed padding-bottom -- overriding the $label-v-spacing bottom
padding that forms.field-label() sets unconditionally on every
<label>. My first-pass conversion to padding-inline-end alone dropped
that override, so the label picked the mixin's padding-bottom back up.
Fixed by adding padding-block-end: 0 alongside padding-inline-end.
Systematically re-audited every other bare "padding"/"margin"-shorthand
bidi() conversion in this file for the same class of bug (a dropped
side silently falls back to some *other* rule's non-zero value rather
than the CSS-initial 0) -- the border-width conversion in _quotes.scss
already covered all four sides explicitly and wasn't affected.

Every conversion verified against a before/after sass compile diff of
both protocol.scss and protocol-components.scss -- confirmed each
logical property produces byte-identical LTR and RTL output to the
physical-plus-[dir=rtl]-override pair it replaced.

Stacked on v23/focus-visible-forms. Part of #1084.

Verified: npm run lint, npm test (47 specs, Firefox + Chrome),
npm run build-docs (523 items, no errors), npm run build-package, and
the before/after compile diff described above all pass.
@stephaniehobson
stephaniehobson added this pull request to stack #1149 October 6, 2026 04:24
@stephaniehobson
stephaniehobson requested a balanced review from Copilot October 6, 2026 17:05
Comment thread assets/sass/protocol/components/forms/_choice.scss

This comment was marked as outdated.

Comment thread assets/sass/protocol/components/_notification-bar.scss Outdated
Comment thread assets/sass/protocol/components/_sticky-promo.scss
@stephaniehobson

This comment was marked as resolved.

@stephaniehobson
stephaniehobson marked this pull request as ready for review October 7, 2026 19:04
@stephaniehobson stephaniehobson changed the title Convert remaining components to CSS logical properties and remove bidi() (#1084) Convert remaining components to CSS logical properties (#1084) Oct 7, 2026
@stephaniehobson
stephaniehobson requested a balanced review from Copilot October 7, 2026 19:06

This comment was marked as outdated.

@stephaniehobson stephaniehobson added the Needs:Review 👋 Ready for Developer Review label Oct 7, 2026
@stephaniehobson
stephaniehobson requested a balanced review from Copilot October 7, 2026 19:51
@stephaniehobson

Copy link
Copy Markdown
Contributor Author

I had a rebase go sideways so I don't have a diff, but this now includes review fixes.

This comment was marked as resolved.

Converts the last 26 @include bidi() call sites across button, breadcrumb,
card, choice, language-switcher, logo, wordmark, modal, notification-bar,
picto, and sticky-promo, plus a handful of plain physical properties
(clear, margin-left) in the same files.

Properties with no logical equivalent (background-position, content,
animation-name) keep an explicit [dir='rtl'] override instead.

bidi() itself is no longer used anywhere internally, but stays available
and exported (includes/mixins/_bidi.scss, @forward 'bidi') for downstream
consumers who still call it directly -- removing it outright is a bigger
migration than we want to force on them. Docs/CHANGELOG/migration guide
now describe it as deprecated, not removed.

Review fixes:
- notification-bar and sticky-promo close buttons: reverted a switch from
  the working `inset-inline-end: 0` to physical `right: 0` plus a new
  [dir='rtl'] override that duplicated an override already present a few
  lines down. Both buttons use @include image-replaced, which forces
  direction: ltr and needs that existing manual [dir='rtl'] +
  inset-inline override to flip correctly -- a plain logical property
  alone doesn't flip under a forced direction.
- choice.scss checkbox checkmark: left -> inset-inline-start, and dropped
  a now-redundant `left: auto` from its [dir='rtl'] override (the
  right-side value, 0.83rem, is intentionally asymmetric, not a mirror).

review fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs:Review 👋 Ready for Developer Review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants